-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Change __rust_no_alloc_shim_is_unstable to be a function #141061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Do you have a link to the problems you are having? |
Yes, sorry, I was intending to add a full write-up to the description once I had done the PR builds to validate that this works with both MinGW and MSVC. I've updated the description now. |
YAML changes were for testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we handle regular #[no_mangle]
statics that doesn't cause those linker warnings/errors?
library/alloc/src/alloc.rs
Outdated
#[rustc_nounwind] | ||
#[rustc_std_internal_symbol] | ||
#[cfg(not(bootstrap))] | ||
fn __rust_no_alloc_shim_is_unstable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the very least this symbol name will need to be changed to prevent UB when someone uses the old definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll rename it.
library/alloc/src/alloc.rs
Outdated
core::ptr::read_volatile(&__rust_no_alloc_shim_is_unstable); | ||
#[cfg(not(bootstrap))] | ||
__rust_no_alloc_shim_is_unstable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would read_volatile(__rust_no_alloc_shim_is_unstable.addr().cast::<u8>())
(using the unstable FnPtr::addr
method) work? Or maybe one of OpenBSD's exploit mitigations would break with that. Do we even care about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work for most platforms.
I'm not sure about OpenBSD, but it would certainly cause issues in Xbox kernel-mode, as it loads binaries as Execute-Only (i.e., they are not readable).
Again, I suspect that the volatile read is more expensive than a tail call to empty function as it may generate load-acquire semantics.
I'm not sure if we could get away with just getting the address of the function and not reading from it? Backends might optimize that away though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe black_box(__rust_no_alloc_shim_is_unstable as fn())
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would read_volatile(__rust_no_alloc_shim_is_unstable.addr().cast::()) (using the unstable FnPtr::addr method) work?
Miri would definitely complain about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
black_box(__rust_no_alloc_shim_is_unstable as fn())
?
Doesn't work: we get the correct behavior where it is required for linking to work, but then the codegen tests fail as the compiler is not able to optimize away allocations.
If the static is exported by one crate and referenced by another, then the compiler knows whether that static will be statically linked or dynamically imported since it can see the dependency chain between the current crate using the static and the declaring crate. If the static is referenced via an |
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
If the static is defined inside an rlib, and we are currently compiling an rlib, rustc can't know if the static will be statically linked or dynamically imported. Only when we are linking the current rlib into something do we know if the other rlib was statically linked or dynamically linked. |
The warning is only emitted if something marked with |
This comment has been minimized.
This comment has been minimized.
cc @ojeda this will require RfL changes when it lands. |
} else if item_name == "__rust_no_alloc_shim_is_unstable_v2" { | ||
// Temporary back compat hack to give people the chance to migrate to | ||
// include #[rustc_std_internal_symbol]. | ||
return "__rust_no_alloc_shim_is_unstable".to_owned(); | ||
return "__rust_no_alloc_shim_is_unstable_v2".to_owned(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case can be removed now. This is a breaking change anyway, so adding #[rustc_std_internal_symbol]
to the definition at the same time should be fine.
@bjorn3 Thanks for the ping! Currently we only use |
Right, forgot about that. In that case you shouldn't need to change anything. |
Good news, then -- thanks for confirming! |
Ping... this is blocking me from fixing Arm64EC again. Can someone please review the PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't get around reviewing this PR until now.
r=me with the #[cfg(bootstrap)]
removed.
This comment has been minimized.
This comment has been minimized.
@bors r+ |
Change __rust_no_alloc_shim_is_unstable to be a function This fixes a long sequence of issues: 1. A customer reported that building for Arm64EC was broken: rust-lang#138541 2. This was caused by a bug in my original implementation of Arm64EC support, namely that only functions on Arm64EC need to be decorated with `#` but Rust was decorating statics as well. 3. Once I corrected Rust to only decorate functions, I started linking failures where the linker couldn't find statics exported by dylib dependencies. This was caused by the compiler not marking exported statics in the generated DEF file with `DATA`, thus they were being exported as functions not data. 4. Once I corrected the way that the DEF files were being emitted, the linker started failing saying that it couldn't find `__rust_no_alloc_shim_is_unstable`. This is because the MSVC linker requires the declarations of statics imported from other dylibs to be marked with `dllimport` (whereas it will happily link to functions imported from other dylibs whether they are marked `dllimport` or not). 5. I then made a change to ensure that `__rust_no_alloc_shim_is_unstable` was marked as `dllimport`, but the MSVC linker started emitting warnings that `__rust_no_alloc_shim_is_unstable` was marked as `dllimport` but was declared in an obj file. This is a harmless warning which is a performance hint: anything that's marked `dllimport` must be indirected via an `__imp` symbol so I added a linker arg in the target to suppress the warning. 6. A customer then reported a similar warning when using `lld-link` (<rust-lang#140176 (comment)>). I don't think it was an implementation difference between the two linkers but rather that, depending on the obj that the declaration versus uses of `__rust_no_alloc_shim_is_unstable` landed in we would get different warnings, so I suppressed that warning as well: rust-lang#140954. 7. Another customer reported that they weren't using the Rust compiler to invoke the linker, thus these warnings were breaking their build: <rust-lang#140176 (comment)>. At that point, my original change was reverted (rust-lang#141024) leaving Arm64EC broken yet again. Taking a step back, a lot of these linker issues arise from the fact that `__rust_no_alloc_shim_is_unstable` is marked as `extern "Rust"` in the standard library and, therefore, assumed to be a foreign item from a different crate BUT the Rust compiler may choose to generate it either in the current crate, some other crate that will be statically linked in OR some other crate that will by dynamically imported. Worse yet, it is impossible while building a given crate to know if `__rust_no_alloc_shim_is_unstable` will statically linked or dynamically imported: it might be that one of its dependent crates is the one with an allocator kind set and thus that crate (which is compiled later) will decide depending if it has any dylib dependencies or not to import `__rust_no_alloc_shim_is_unstable` or generate it. Thus, there is no way to know if the declaration of `__rust_no_alloc_shim_is_unstable` should be marked with `dllimport` or not. There is a simple fix for all this: there is no reason `__rust_no_alloc_shim_is_unstable` must be a static. It needs to be some symbol that must be linked in; thus, it could easily be a function instead. As a function, there is no need to mark it as `dllimport` when dynamically imported which avoids the entire mess above. There may be a perf hit for changing the `volatile load` to be a `tail call`, so I'm happy to change that part back (although I question what the codegen of a `volatile load` would look like, and if the backend is going to try to use load-acquire semantics). Build with this change applied BEFORE rust-lang#140176 was reverted to demonstrate that there are no linking issues with either MSVC or MinGW: <https://github.com/rust-lang/rust/actions/runs/15078657205> Incidentally, I fixed `tests/run-make/no-alloc-shim` to work with MSVC as I needed it to be able to test locally (FYI for rust-lang#128602) r? `@bjorn3` cc `@jieyouxu`
// "__rust_no_alloc_shim_is_unstable" | ||
let val = ImmTy::from_int(0, ecx.machine.layouts.u8); // always 0, value does not matter | ||
Self::alloc_extern_static(ecx, "__rust_no_alloc_shim_is_unstable", val)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After removing this symbol you'll also have to adjust tests/pass/alloc-access-tracking.rs
and decrement the alloc id there by 1.
PR CI would have caught this, but strangely did not run here it seems.
@bors r- |
This fixes a long sequence of issues:
arm64ec-pc-windows-msvc
#138541#
but Rust was decorating statics as well.DATA
, thus they were being exported as functions not data.__rust_no_alloc_shim_is_unstable
. This is because the MSVC linker requires the declarations of statics imported from other dylibs to be marked withdllimport
(whereas it will happily link to functions imported from other dylibs whether they are markeddllimport
or not).__rust_no_alloc_shim_is_unstable
was marked asdllimport
, but the MSVC linker started emitting warnings that__rust_no_alloc_shim_is_unstable
was marked asdllimport
but was declared in an obj file. This is a harmless warning which is a performance hint: anything that's markeddllimport
must be indirected via an__imp
symbol so I added a linker arg in the target to suppress the warning.lld-link
(Fix linking statics on Arm64EC #140176 (comment)). I don't think it was an implementation difference between the two linkers but rather that, depending on the obj that the declaration versus uses of__rust_no_alloc_shim_is_unstable
landed in we would get different warnings, so I suppressed that warning as well: [win] Ignore MSVC linker warning 4217 #140954.Taking a step back, a lot of these linker issues arise from the fact that
__rust_no_alloc_shim_is_unstable
is marked asextern "Rust"
in the standard library and, therefore, assumed to be a foreign item from a different crate BUT the Rust compiler may choose to generate it either in the current crate, some other crate that will be statically linked in OR some other crate that will by dynamically imported.Worse yet, it is impossible while building a given crate to know if
__rust_no_alloc_shim_is_unstable
will statically linked or dynamically imported: it might be that one of its dependent crates is the one with an allocator kind set and thus that crate (which is compiled later) will decide depending if it has any dylib dependencies or not to import__rust_no_alloc_shim_is_unstable
or generate it. Thus, there is no way to know if the declaration of__rust_no_alloc_shim_is_unstable
should be marked withdllimport
or not.There is a simple fix for all this: there is no reason
__rust_no_alloc_shim_is_unstable
must be a static. It needs to be some symbol that must be linked in; thus, it could easily be a function instead. As a function, there is no need to mark it asdllimport
when dynamically imported which avoids the entire mess above.There may be a perf hit for changing the
volatile load
to be atail call
, so I'm happy to change that part back (although I question what the codegen of avolatile load
would look like, and if the backend is going to try to use load-acquire semantics).Build with this change applied BEFORE #140176 was reverted to demonstrate that there are no linking issues with either MSVC or MinGW: https://github.com/rust-lang/rust/actions/runs/15078657205
Incidentally, I fixed
tests/run-make/no-alloc-shim
to work with MSVC as I needed it to be able to test locally (FYI for #128602)r? @bjorn3
cc @jieyouxu